Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Upgrade Synapse (1.3.1 -> 1.4.0) #275

Merged
merged 4 commits into from
Oct 3, 2019

Conversation

aaronraimist
Copy link
Contributor

@aaronraimist aaronraimist commented Oct 3, 2019

#273 should happen at the same time

Comment on lines +937 to 948
# Note: This option is deprecated. Since v0.99.4, Synapse has tracked which identity
# server a 3PID has been bound to. For 3PIDs bound before then, Synapse runs a
# background migration script, informing itself that the identity server all of its
# 3PIDs have been bound to is likely one of the below.
#
# As of Synapse v1.4.0, all other functionality of this option has been deprecated, and
# it is now solely used for the purposes of the background migration script, and can be
# removed once it has run.
{% if matrix_synapse_trusted_third_party_id_servers|length > 0 %}
trusted_third_party_id_servers:
{{ matrix_synapse_trusted_third_party_id_servers|to_nice_yaml }}
{% endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you going to do with the identity server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't need an identity server anymore. I would continue to run mxisd but I really don't have a purpose for it after Synapse 1.4.0 and it doesn't support the unbind API which leads to a broken experience.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting the impression that we should still keep it for "background migration" purposes.
Or maybe that migration script had already been executed as part of a previous release..?

It doesn't hurt to keep trusted_third_party_id_servers around, at least initially.


As for disabling our identity server, I think I won't need it as well.

However, if we disable our identity server right now, matrix_synapse_trusted_third_party_id_servers would default to matrix_synapse_id_servers_public, which is ['matrix.org', 'vector.im']. The comment says this trusted_third_party_id_servers setting isn't used for anything so maybe that's okay.


There are 2 other more major side-effects of disabling our identity server and having matrix_synapse_trusted_third_party_id_servers fall back to ['matrix.org', 'vector.im'] for us.

The first server in that matrix_synapse_trusted_third_party_id_servers list (in this case, 'matrix.org') ends up in matrix_identity_server_url.

This is used for riot-web's default_is_url setting (via the matrix_riot_web_default_is_url variable).
So, suddenly, riot-web starts pointing to matrix.org as a default server. That's probably not good.

Furthermore, matrix_identity_server_url is used in one other place -- the generation of the /.well-known/matrix/client file. All other clients (including Riot on mobile devices) which consult the /.well-known/matrix/client file will see that our server recommends talking to matrix.org as an identity server. That's probably not good as well.


So I think we should keep running an identity server.. Or set these defaults to something else (null?).

I'm not sure if clients like null values now or whether they decide to fall back to something else in such a case (matrix.org again?).

If mobile applications really do respect null, that means they disable their "Find contacts" feature. While I find that feature largely useless (and would never share my contacts with any app), we may wish to find a way to get it working (that is, by running an identity server by default?)

In any case, I'm not sure how useful such a feature is when pointed to our own self-hosted identity server. Delegation between identity servers only works between mxisd servers that are authoritative for their own domain. That's nice, but in the real world, most users are unfortunately using email addresses like gmail.com, etc. In such a centralized world, we may decide to kill the identity server thing (by default) for most users, and leave it as an option to enable for those who need it.

Still, lots of things to investigate before taking the right action..

Copy link
Contributor Author

@aaronraimist aaronraimist Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think we should keep running an identity server.. Or set these defaults to something else (null?).

I got an Ansible error message if I set it to null but it is happy to accept matrix_synapse_trusted_third_party_id_servers: []

I agree that we should keep it around for a while while people upgrade. The background migration happens as soon as you upgrade to 1.4.0 is my understanding.

For me the background migration process did the wrong thing since I initially set my server up a few years ago before this playbook or mxisd existed. When the background migration process ran it set the identity server to matrix.raim.ist which meant I couldn't unbind them and remove them from the actual server where they were (vector.im). It was easy enough for me to UPDATE user_threepid_id_server SET id_server = 'vector.im' WHERE user_id = '@aaron:raim.ist'; in the database which allowed me to remove them from vector.im. Most people are probably not in that camp though so we probably don't need to mention it in the docs.

This is used for riot-web's default_is_url setting (via the matrix_riot_web_default_is_url variable).
So, suddenly, riot-web starts pointing to matrix.org as a default server. That's probably not good.

If you set it to matrix_synapse_trusted_third_party_id_servers: [] then the riot config looks like:
"default_is_url": "",
and /.well-known/matrix/client looks like

	"m.identity_server": {
		"base_url": ""
	}

You can probably just modify those to remove those sections when matrix_synapse_trusted_third_party_id_servers is set to []. It does seem to work fine though.
Screen Shot 2019-10-03 at 12 04 02 PM
Riot doesn't seem to ask for an identity server on login anymore.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also did something weird with one of the email address bound to my account. I think it was validated by the same server, but I'm not sure when and how that happened..

Unbinding was not possible until I deleted that specific entry from user_threepid_id_server. I believe that not having any entry there makes Synapse think it's responsible for validating the 3pid and would just drop it without complaining. I'm fine with that, so I went that route.

Regular users would probably not experience this though..


Thanks for confirming riot-web's behavior with:

"m.identity_server": {
	"base_url": ""
}

If Riot/Android and Riot/iOS also behave well, we can try switching to that.
I'm curious what their "Contacts" tab would do in such a case though. I can probably test tomorrow or something.

Copy link
Contributor Author

@aaronraimist aaronraimist Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Riot iOS does not appear to like that on 0.9.5. When I try to use the /.well-known/matrix/client, it says Invalid homeserver discovery response. Maybe it will behave better in the next version, the stuff about disconnecting from an intensity server hasn't been implemented yet in the mobile apps element-hq/element-ios#2746.

If I try to delete https://vector.im from the identity server URL field, Riot iOS just fills it back in. Setting it to localhost seems to work but I haven't verified that it isn't connecting to vector.im.

@spantaleev spantaleev marked this pull request as ready for review October 3, 2019 16:35
@spantaleev spantaleev merged commit 19fb96f into spantaleev:master Oct 3, 2019
@spantaleev
Copy link
Owner

I've merged it like this.

Thank you, @aaronraimist! 👍

We should proceed thinking about whether to kill off the identity server by default or not. We can do this separately.

@aaronraimist aaronraimist deleted the synapse-1.4.0 branch October 3, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants